feat: wire fdv1-fallback capability into node-client, browser, and react-native contract-test entities - #1858
Conversation
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@launchdarkly/browser size report |
|
@launchdarkly/js-client-sdk size report |
| # RN's forked EventSource (react-native-sse) fires `open` too late (from | ||
| # onreadystatechange at readyState DONE, not from onprogress at LOADING) and never | ||
| # populates headers on error events, so FDv1 fallback directives carried on stream | ||
| # response headers don't reach the SDK in time (or at all, on the error path). | ||
| streaming/fdv2/FDv1 fallback directive/directive on streaming error engages FDv1 fallback | ||
| streaming/fdv2/FDv1 fallback directive/directive on streaming success applies payload then engages FDv1 | ||
| streaming/fdv2/FDv1 fallback directive/directive without FDv1 fallback configured halts the data system | ||
| tags/FDv1 fallback directive requests |
There was a problem hiding this comment.
This is tracked in a separate ticket
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| function translateModeDefinition( | ||
| modeDef: SDKConfigModeDefinition, | ||
| fdv1Fallback?: SDKConfigPollingParams | null, | ||
| ): ModeDefinition { |
There was a problem hiding this comment.
🟡 Fallback polling settings are ignored when tests use custom connection modes
The fallback polling settings sent by the test harness are only passed along in the default single-mode path (translateModeDefinition(modeDef, options.dataSystem.fdv1Fallback) at packages/sdk/browser/contract-tests/entity/src/ClientEntity.ts:167) and are dropped for every custom connection mode, so those runs silently use built-in defaults instead of the values the test asked for.
Impact: Tests that combine custom connection modes with fallback settings exercise the wrong endpoint and interval, producing misleading pass/fail results.
How the fallback config is lost in the custom-connection-mode branch
options.dataSystem.fdv1Fallback is a sibling of options.dataSystem.connectionModeConfig (packages/tooling/contract-test-utils/src/types/ConfigParams.ts:30), but in the connectionModeConfig branch each mode is translated with translateModeDefinition(modeDef) and no fallback argument (packages/sdk/browser/contract-tests/entity/src/ClientEntity.ts:150, packages/sdk/node-client/contract-tests/src/sdkClientEntity.ts:156, packages/sdk/react-native/contract-tests/entity/src/ClientEntity.ts:149). Without fdv1Fallback on the ModeDefinition, the SDK falls back to MODE_TABLE defaults (300s poll interval, default polling endpoint) in packages/shared/sdk-client/src/datasource/FDv2DataManagerBase.ts.
Additionally, the new code only emits the fallback config when fdv1Fallback?.baseUri is truthy, so a harness config supplying only pollIntervalMs is discarded entirely. The server-side entity applies the fallback whenever the object is present (packages/sdk/server-node/contract-tests/src/sdkClientEntity.ts), which is the more faithful translation.
Was this helpful? React with 👍 or 👎 to provide feedback.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f296bbf. Configure here.
| initialConnectionMode: 'streaming', | ||
| }; | ||
| dataSystem.connectionModes = { | ||
| streaming: translateModeDefinition(modeDef), |
There was a problem hiding this comment.
FDv1 fallback skipped for custom modes
Medium Severity
translateModeDefinition now accepts dataSystem.fdv1Fallback, but the connectionModeConfig.customConnectionModes path still calls it without that argument. Harness configs that combine custom modes with fdv1Fallback keep losing the dedicated FDv1 polling URI to applyEndpointOverrides, the same failure this PR fixes on the top-level initializers/synchronizers path.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit f296bbf. Configure here.
f296bbf to
519d71e
Compare
| if (fdv1Fallback?.baseUri) { | ||
| return { | ||
| initializers, | ||
| synchronizers, | ||
| fdv1Fallback: { | ||
| ...(fdv1Fallback.pollIntervalMs != null && { | ||
| pollInterval: fdv1Fallback.pollIntervalMs / 1000, | ||
| }), | ||
| endpoints: { pollingBaseUri: fdv1Fallback.baseUri }, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🟡 Fallback polling settings are thrown away when no fallback address is given
The fallback polling settings are only kept when an address is present (if (fdv1Fallback?.baseUri) at packages/sdk/browser/contract-tests/entity/src/ClientEntity.ts:80), so a configuration that supplies only a polling frequency is silently dropped.
Impact: Test runs that configure the fallback with just a poll interval get default behavior instead of the requested one, producing confusing failures.
Condition gating on baseUri discards pollIntervalMs-only fallback config
translateModeDefinition returns the mode without any fdv1Fallback whenever baseUri is unset, even if pollIntervalMs was provided. The underlying SDK type FDv1FallbackConfig (packages/shared/sdk-client/src/api/datasource/DataSourceEntry.ts:78-83) makes both pollInterval and endpoints optional, so a fallback with only pollInterval is valid and should be forwarded. The same pattern is duplicated in packages/sdk/node-client/contract-tests/src/sdkClientEntity.ts:84-95 and packages/sdk/react-native/contract-tests/entity/src/ClientEntity.ts:76-87.
Prompt for agents
In translateModeDefinition (packages/sdk/browser/contract-tests/entity/src/ClientEntity.ts, packages/sdk/node-client/contract-tests/src/sdkClientEntity.ts, packages/sdk/react-native/contract-tests/entity/src/ClientEntity.ts) the fdv1Fallback block is only emitted when fdv1Fallback.baseUri is set. Since FDv1FallbackConfig accepts pollInterval without endpoints, a harness config specifying only pollIntervalMs is silently ignored. Consider emitting fdv1Fallback whenever the object is present, including endpoints only when baseUri exists.
Was this helpful? React with 👍 or 👎 to provide feedback.
f296bbf to
6f35292
Compare
6f35292 to
9c3d7dc
Compare
…act-native contract-test entities
…ontract-test entity
9c3d7dc to
8a0e71d
Compare
1 participant
Summary
Wires the harness's
dataSystem.fdv1Fallback.baseUriinto the node-client, browser, and react-native contract-test entities so the FDv1 fallback synchronizer connects to the harness's dedicated FDv1 polling endpoint instead of the polling initializer's endpoint (whichapplyEndpointOverridesotherwise overwrites it with).Completes contract-test coverage for the FDv1 fallback/recovery behavior built across this stack.
Note
Overview
Wires harness
dataSystem.fdv1Fallback(polling base URI and interval) into browser, node-client, and React Native contract-test entities viatranslateModeDefinition, so the FDv1 fallback synchronizer uses the harness’s dedicated endpoint instead of initializer polling URIs overwritten byapplyEndpointOverrides.Advertises the
fdv1-fallbackcapability (and nodeclient-event-source-http-errors) to the test harness; addsSDKConfigDataSystem.fdv1Fallbackin contract-test-utils. Browser and React Native suppress FDv1 directive-on-stream scenarios where EventSource cannot expose response headers.In shared
FDv2DataSource,handleFdv1Fallbackskips re-invoking fallback when the active synchronizer is already the FDv1 slot (isCurrentSynchronizerFDv1Fallback), with a regression test for SDK-2617.Reviewed by Cursor Bugbot for commit 8a0e71d. Bugbot is set up for automated code reviews on this repo. Configure here.